fix issue with .c in folder name #126

trollkarlen · 2023-05-09T08:44:01Z

put doom hack under doom define, to use run with -d doom

related to error #124 but not a total fix this just fixes so a wrapper can be created.
May break doom tests but not when the new define is set.

trollkarlen · 2023-05-09T08:57:46Z

Should I change doom to a argument instead of a define, so it can be a runtime not a compile time patch ?
-doom

trollkarlen · 2023-05-09T09:07:50Z

Maybe the same for trace_verbose ?
Will not change until i get your input.

ArtemkaKun · 2023-05-09T09:49:47Z

I don't know if C2V should have different code for different projects

trollkarlen · 2023-05-09T12:39:47Z

It has already :)

So make doom and trace_verbose arguments instead ?

ArtemkaKun · 2023-05-09T12:41:04Z

I mean that doom specific hack should be removed and universal fix should be implemented

trollkarlen · 2023-05-09T12:49:48Z

But thats a bigger task than fixing a issue in the code, and if we put that on every bug fix then there will be no bug fixes :/

ArtemkaKun · 2023-05-09T12:55:40Z

In my opinion it's better to made a solid change one time, than made a few hacky quick fixes and introduce more complexity and potential bugs into the project.

In this case, what if tomorrow a new guy will come and said "my XYZ project requites specific changes to make it work", would we introduce new argument -XYZ that should be used for this specific project only? I don't think this is a good idea.

I appreciate your contribution, we just need to do it in another way.

Have you investigated why Doom requires that hack?

trollkarlen · 2023-05-09T13:13:28Z

In my opinion it's better to made a solid change one time, than made a few hacky quick fixes and introduce more complexity and potential bugs into the project.

In this case, what if tomorrow a new guy will come and said "my XYZ project requites specific changes to make it work", would we introduce new argument -XYZ that should be used for this specific project only? I don't think this is a good idea.

This also just cancels that quick fix, so its not that its a new XYZ fix.

I appreciate your contribution, we just need to do it in another way.

Second it cant be made an argument easily because the some of the test relies on a bash script(https://github.com/vlang/doom/blob/master/build_whole_project.sh) thats not located in this git which is strange in it self.
So it need to be a compile time fix, for it to not get out of hand work wise.

Have you investigated why Doom requires that hack?

I guess this(c2v) and was written by someone that knows v and c very well from the beginning, and that person left it there for a reason. And that reason not being it is a easy fix or that maybe it require a change in doom.

trollkarlen · 2023-05-09T13:17:33Z

So i think to move forward the best is to take changes incrementally and not force this fix to change all doom related issues.
But I can write a issue doom hacks in c2v so that can be tracked, separately.

ArtemkaKun · 2023-05-09T13:25:44Z

Again, I don't think that C2V code should depends on project it translates, neither project, that works, should be changed to be able to be translated by C2V.

We can't introduce copile time things like

if DOOM {
   
} elseif X {

} elseif Y {

} else {

}

The original C2V code was created by @medvednikov , maybe he will help to understand why this Doom specific code is here and how it can be removed.

trollkarlen · 2023-05-09T14:16:26Z

I changed the pull to not include the fix for doom src folder and let others fall on the same branch :/
So it should be clean now

ArtemkaKun · 2023-05-09T15:28:41Z

Got it. I hope issue with Doom will be fixed somewhere in future.

Can you please explain to me how your fix works? I nit quite understand what your changes do

trollkarlen · 2023-05-09T16:11:14Z

I tried to convert xml.c project to v and then i discovered that if you got .c in any folder name its also exchanged to .json so the change makes sure it only replace the last .c to . json and not all of them.

ArtemkaKun · 2023-05-09T16:13:38Z

Do you think you could implement a test for this case?

trollkarlen · 2023-05-09T17:21:13Z

Sure should be easy, except i need to avoid the doom hack 😁

Create anything in a ./src folder will make it change to that folder and fail because it cant find the file its trying to parse 😁

ArtemkaKun · 2023-05-09T17:40:08Z

Not sure how this is connected to src/ folder but whatever. Seems we don't have an infrastructure for that kind of tests

ArtemkaKun · 2023-05-09T17:42:23Z

I will try to review it asap and will merge if everything is ok

trollkarlen · 2023-05-10T07:09:06Z

Test added, the same bug existed in the test runner :)

trollkarlen · 2023-05-10T07:32:17Z

The hack added for doom that with chdir to 'src' is only seen if you dont have absolute paths. Just for reference.
I tried to create a failing testcase for this but the test runner runs with absolute paths on the conversion files.

ArtemkaKun · 2023-05-10T07:38:12Z

So the hack with DOOM is needed only for CI checks?

trollkarlen · 2023-05-10T07:49:41Z

So the hack with DOOM is needed only for CI checks?
No the opposite.

When it does the conversion if it finds '/src/' somewhere on the path it chdir to that location to i guess find a headerfile that in the wrong location. So if the path was relative from the start then the new relative path will be wrong.

Example:

v translate banana/src/test.c

it will fail because it will chdir to banana/src and then try to convert the file banana/src/test.c but that file does not exist.
But if you have the abs path it works even thou it chdir to banana/src because you started it with an absolute path.

Full test and example:

$ mkdir tests/src
$ cp -a tests/test.c tests/src/
$ v run . tests/src/test.c/1.hello.c
C to V translator 0.3.1
  translating tests/src/test.c/1.hello.c ... sh: line 1: tests/src/test.c/1.hello.json: No such file or directory

The file tests/src/test.c/1.hello.c could not be parsed as a C source file.
$ v run . $(pwd)/tests/src/test.c/1.hello.c
C to V translator 0.3.1
  translating $HOME/c2v/tests/src/test.c/1.hello.c ... Reformatted file: $HOME/c2v/tests/src/test.c/1.hello.v
 took    94 ms ; output .v file: test.c/1.hello.v
Translated   1 files in    94 ms.

trollkarlen · 2023-05-10T07:50:11Z

Either way this should be ready for merge

$ git checkout 9a5bacb6bc6d5edf578b48890079245198c6af3a -b test-works   # this pull request                                                                                                                                      
Switched to a new branch 'test-works'                                                                                                                                                                    
$ cp -a tests/test.c .              # park the test from the new commit                                                                                                                                                                                
$ git checkout 811a8b7c320d1fda9a23d4a95ed5f84ccc1f2537 -b test-fail            # master and base of this pull                                                                                                                              
$ cp -a test.c/ tests/test.c      # get the test from the new commit                                                                   
$ v -g run tests/run_tests.vsh
building c2v...
done
Reformatted file: $HOME/dev/c2v/tests/1.hello.v
Already formatted file: $HOME/dev/c2v/tests/1.hello.v
$HOME/dev/c2v/tests/1.hello.c...  OK
Reformatted file: $HOME/dev/c2v/tests/11.enum_default.v
Already formatted file: $HOME/dev/c2v/tests/11.enum_default.v
$HOME/dev/c2v/tests/11.enum_default.c...  OK
Reformatted file: $HOME/dev/c2v/tests/12.if_stmt.v
Already formatted file: $HOME/dev/c2v/tests/12.if_stmt.v
$HOME/dev/c2v/tests/12.if_stmt.c...  OK
Reformatted file: $HOME/dev/c2v/tests/13.switch.v
Already formatted file: $HOME/dev/c2v/tests/13.switch.v
$HOME/dev/c2v/tests/13.switch.c...  OK
Reformatted file: $HOME/dev/c2v/tests/14.default.v
Already formatted file: $HOME/dev/c2v/tests/14.default.v
$HOME/dev/c2v/tests/14.default.c...  OK
Reformatted file: $HOME/dev/c2v/tests/15.multi_var_decl.v
Already formatted file: $HOME/dev/c2v/tests/15.multi_var_decl.v
$HOME/dev/c2v/tests/15.multi_var_decl.c...  OK
Reformatted file: $HOME/dev/c2v/tests/2.if.v
Already formatted file: $HOME/dev/c2v/tests/2.if.v
$HOME/dev/c2v/tests/2.if.c...  OK
Reformatted file: $HOME/dev/c2v/tests/3.if_switch_enum.v
Already formatted file: $HOME/dev/c2v/tests/3.if_switch_enum.v
$HOME/dev/c2v/tests/3.if_switch_enum.c...  OK
Reformatted file: $HOME/dev/c2v/tests/4.for.v
Already formatted file: $HOME/dev/c2v/tests/4.for.v
$HOME/dev/c2v/tests/4.for.c...  OK
Reformatted file: $HOME/dev/c2v/tests/5.struct.v
Already formatted file: $HOME/dev/c2v/tests/5.struct.v
$HOME/dev/c2v/tests/5.struct.c...  OK
Reformatted file: $HOME/dev/c2v/tests/6.types.v                                                                                                                                                  
Already formatted file: $HOME/dev/c2v/tests/6.types.v                                                                                                                                            
$HOME/dev/c2v/tests/6.types.c...  OK                                                                                                                                                             
sh: line 1: $HOME/dev/c2v/tests/src/test.json/1.hello.json: No such file or directory                                                                                                            
                                                                                                                                                                                                         
The file $HOME/dev/c2v/tests/src/test.c/1.hello.c could not be parsed as a C source file.                                                                                                        
$HOME/dev/c2v/tests/src/test.c/1.hello.c...  FAIL                                                                                                                                                
$ rm -fr tests/test.c   # remove to not get conflict                                                                                                                                                                                  
$ git checkout test-works                                                                                                                                                                                
Switched to branch 'test-works'                                                                                                                                                                          
$ v -g run tests/run_tests.vsh                                                                                                                                                                           
building c2v...                                                                                                                                                                                          
done
Reformatted file: $HOME/dev/c2v/tests/1.hello.v
Already formatted file: $HOME/dev/c2v/tests/1.hello.v
$HOME/dev/c2v/tests/1.hello.c...  OK
Reformatted file: $HOME/dev/c2v/tests/11.enum_default.v
Already formatted file: $HOME/dev/c2v/tests/11.enum_default.v
$HOME/dev/c2v/tests/11.enum_default.c...  OK
Reformatted file: $HOME/dev/c2v/tests/12.if_stmt.v
Already formatted file: $HOME/dev/c2v/tests/12.if_stmt.v
$HOME/dev/c2v/tests/12.if_stmt.c...  OK
Reformatted file: $HOME/dev/c2v/tests/13.switch.v
Already formatted file: $HOME/dev/c2v/tests/13.switch.v
$HOME/dev/c2v/tests/13.switch.c...  OK
Reformatted file: $HOME/dev/c2v/tests/14.default.v
Already formatted file: $HOME/dev/c2v/tests/14.default.v
$HOME/dev/c2v/tests/14.default.c...  OK
Reformatted file: $HOME/dev/c2v/tests/15.multi_var_decl.v
Already formatted file: $HOME/dev/c2v/tests/15.multi_var_decl.v
$HOME/dev/c2v/tests/15.multi_var_decl.c...  OK
Reformatted file: $HOME/dev/c2v/tests/2.if.v
Already formatted file: $HOME/dev/c2v/tests/2.if.v
$HOME/dev/c2v/tests/2.if.c...  OK
Reformatted file: $HOME/dev/c2v/tests/3.if_switch_enum.v
Already formatted file: $HOME/dev/c2v/tests/3.if_switch_enum.v
$HOME/dev/c2v/tests/3.if_switch_enum.c...  OK
Reformatted file: $HOME/dev/c2v/tests/4.for.v
Already formatted file: $HOME/dev/c2v/tests/4.for.v
$HOME/dev/c2v/tests/4.for.c...  OK
Reformatted file: $HOME/dev/c2v/tests/5.struct.v
Already formatted file: $HOME/dev/c2v/tests/5.struct.v
$HOME/dev/c2v/tests/5.struct.c...  OK
Reformatted file: $HOME/dev/c2v/tests/6.types.v
Already formatted file: $HOME/dev/c2v/tests/6.types.v
$HOME/dev/c2v/tests/6.types.c...  OK
Reformatted file: $HOME/dev/c2v/tests/src/test.c/1.hello.v
Already formatted file: $HOME/dev/c2v/tests/src/test.c/1.hello.v
$HOME/dev/c2v/tests/src/test.c/1.hello.c...  OK
Reformatted file: $HOME/dev/c2v/tests/test.c/1.hello.v
Already formatted file: $HOME/dev/c2v/tests/test.c/1.hello.v
$HOME/dev/c2v/tests/test.c/1.hello.c...  OK
Reformatted file: $HOME/dev/c2v/tests/10.jni.v
$HOME/dev/c2v/tests/10.jni.h...  OK
Reformatted file: $HOME/dev/c2v/tests/7.api_types.v
$HOME/dev/c2v/tests/7.api_types.h...  OK
Reformatted file: $HOME/dev/c2v/tests/8.simple_func_header.v
$HOME/dev/c2v/tests/8.simple_func_header.h...  OK
Reformatted file: $HOME/dev/c2v/tests/9.func_declaration.v
$HOME/dev/c2v/tests/9.func_declaration.h...  OK

ArtemkaKun · 2023-05-10T08:06:20Z

So maybe we can make sure that path, provided in the parameters for c2v command, is always absolute? For example, we can use os.is_abs_path and os.abs_path for that

trollkarlen · 2023-05-10T08:14:17Z

So maybe we can make sure that path, provided in the parameters for c2v command, is always absolute? For example, we can use os.is_abs_path and os.abs_path for that

Yes, thats a possible workaround but not for this pull request but for #127.
But it can generate other issues due to the chdir other header files can maybe not be found in other projects.

ArtemkaKun · 2023-05-10T08:31:55Z

Ok, got it. So I will merge that PR after a codereview

tests/run_tests.vsh

src/c2v.v

Signed-off-by: Robert Marklund <[email protected]>

trollkarlen · 2023-05-11T09:56:25Z

It wasn't so easy to create a utils file and have that imported in the test runner so it became like this instead.
There was also a copy paste bug(or waste) in the test file that got found and resolved this way.

trollkarlen force-pushed the master branch from 33ee1c1 to 39d9f75 Compare May 9, 2023 13:20

trollkarlen force-pushed the master branch from 39d9f75 to c518dcb Compare May 9, 2023 14:14

trollkarlen force-pushed the master branch from c518dcb to 9a5bacb Compare May 10, 2023 07:08

trollkarlen mentioned this pull request May 10, 2023

remove print on optional else after if block #128

Merged

ArtemkaKun suggested changes May 10, 2023

View reviewed changes

tests/run_tests.vsh Outdated Show resolved Hide resolved

src/c2v.v Outdated Show resolved Hide resolved

src/c2v.v Show resolved Hide resolved

fix issue with ".c" in folder name

3e0ff82

Signed-off-by: Robert Marklund <[email protected]>

trollkarlen force-pushed the master branch from 9a5bacb to 3e0ff82 Compare May 11, 2023 09:54

ArtemkaKun approved these changes May 11, 2023

View reviewed changes

ArtemkaKun added 2 commits May 11, 2023 12:40

Add NOTE comment to replace_file_extension (c2v.v)

f75abce

Add NOTE comment to replace_file_extension (run_tests.vsh)

8ec2cba

ArtemkaKun merged commit d84d969 into vlang:master May 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix issue with .c in folder name #126

fix issue with .c in folder name #126

trollkarlen commented May 9, 2023

trollkarlen commented May 9, 2023

trollkarlen commented May 9, 2023

ArtemkaKun commented May 9, 2023

trollkarlen commented May 9, 2023

ArtemkaKun commented May 9, 2023

trollkarlen commented May 9, 2023

ArtemkaKun commented May 9, 2023 •

edited

Loading

trollkarlen commented May 9, 2023

trollkarlen commented May 9, 2023 •

edited

Loading

ArtemkaKun commented May 9, 2023

trollkarlen commented May 9, 2023

ArtemkaKun commented May 9, 2023

trollkarlen commented May 9, 2023

ArtemkaKun commented May 9, 2023

trollkarlen commented May 9, 2023 •

edited

Loading

ArtemkaKun commented May 9, 2023

ArtemkaKun commented May 9, 2023

trollkarlen commented May 10, 2023

trollkarlen commented May 10, 2023

ArtemkaKun commented May 10, 2023

trollkarlen commented May 10, 2023

trollkarlen commented May 10, 2023 •

edited

Loading

ArtemkaKun commented May 10, 2023

trollkarlen commented May 10, 2023

ArtemkaKun commented May 10, 2023

trollkarlen commented May 11, 2023

fix issue with .c in folder name #126

fix issue with .c in folder name #126

Conversation

trollkarlen commented May 9, 2023

trollkarlen commented May 9, 2023

trollkarlen commented May 9, 2023

ArtemkaKun commented May 9, 2023

trollkarlen commented May 9, 2023

ArtemkaKun commented May 9, 2023

trollkarlen commented May 9, 2023

ArtemkaKun commented May 9, 2023 • edited Loading

trollkarlen commented May 9, 2023

trollkarlen commented May 9, 2023 • edited Loading

ArtemkaKun commented May 9, 2023

trollkarlen commented May 9, 2023

ArtemkaKun commented May 9, 2023

trollkarlen commented May 9, 2023

ArtemkaKun commented May 9, 2023

trollkarlen commented May 9, 2023 • edited Loading

ArtemkaKun commented May 9, 2023

ArtemkaKun commented May 9, 2023

trollkarlen commented May 10, 2023

trollkarlen commented May 10, 2023

ArtemkaKun commented May 10, 2023

trollkarlen commented May 10, 2023

trollkarlen commented May 10, 2023 • edited Loading

ArtemkaKun commented May 10, 2023

trollkarlen commented May 10, 2023

ArtemkaKun commented May 10, 2023

trollkarlen commented May 11, 2023

ArtemkaKun commented May 9, 2023 •

edited

Loading

trollkarlen commented May 9, 2023 •

edited

Loading

trollkarlen commented May 9, 2023 •

edited

Loading

trollkarlen commented May 10, 2023 •

edited

Loading